Skip to content

fix(wallet): use renamed handler for locked coins signal - #7667

Merged
knst merged 1 commit into
dashpay:developfrom
PastaPastaPasta:fix/wallet-locked-coins-handler
Sep 8, 2026
Merged

fix(wallet): use renamed handler for locked coins signal#7667
knst merged 1 commit into
dashpay:developfrom
PastaPastaPasta:fix/wallet-locked-coins-handler

Conversation

@PastaPastaPasta

@PastaPastaPasta PastaPastaPasta commented Sep 8, 2026

Copy link
Copy Markdown
Member

Issue being fixed or feature implemented

Merging #7648 and #7602 together broke wallet-enabled builds on develop: wallet/interfaces.cpp:823:16: error: use of undeclared identifier 'MakeHandler'. #7648 renamed the signal-handler factory to MakeSignalHandler as part of bitcoin#26298, while #7602 introduced handleLockedCoinsChanged using the old name.

What was done?

Update the locked-coins notification to use MakeSignalHandler, matching the other wallet signal handlers and preserving connection cleanup. This was the only remaining MakeHandler reference in src.

How Has This Been Tested?

On macOS arm64 with the prebuilt depends dependencies, wallet and Qt enabled:

  • Reproduced the exact compiler error before the fix with make -C src wallet/libbitcoin_wallet_a-interfaces.o.
  • Full make -j15 passed, including dashd, dash-qt, and the test binaries.
  • ./src/test/test_dash --run_test=wallet_tests --report_level=short: 20 cases and 626 assertions passed.
  • QT_QPA_PLATFORM=minimal ./src/qt/test/test_dash-qt: 90 passed, 1 skipped, 0 failed.
  • COMMIT_RANGE=upstream/develop..HEAD python3 test/lint/lint-whitespace.py and git diff --check passed.

No new test is needed for this one-line API rename repair; compiling the affected translation unit directly reproduces the regression and verifies the fix.

Breaking Changes

None.

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation
  • I have assigned this pull request to a milestone

This pull request was created by Codex.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 8, 2026 •
edited
Loading

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-08T14:43:08.936419Z 71410d5 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@coderabbitai

coderabbitai Bot commented Sep 8, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: e59749fb-3af3-4767-b0cd-2ff2c4f9677f

📥 Commits

Reviewing files that changed from the base of the PR and between bb52a98 and 71410d5.

📒 Files selected for processing (1)
  • src/wallet/interfaces.cpp

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

Walkthrough

WalletImpl::handleLockedCoinsChanged now uses MakeSignalHandler when connecting to NotifyLockedCoinsChanged. This matches the handler construction used by other wallet signal connections.

Priority: ⬇️ Low — Defer this wallet handler rename because it is a one-line compatibility fix for the locked-coins signal with no broader product-surface impact.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to 71410

The locked-coins notification now uses the renamed signal-handler factory, preserving connection cleanup behavior and restoring wallet-enabled build compatibility. No current merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the wallet signal-handler rename that fixes the broken locked-coins notification build.
Description check ✅ Passed The description directly explains the build regression, the MakeSignalHandler fix, preserved cleanup behavior, and validation results.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@thepastaclaw

thepastaclaw commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

🕓 Queued for automated review — 65th in line, estimated start in ~95 h (commit 71410d5)
Estimated review time once started: ~2.9 h (two-phase automated review; median of recent runs).

  • Request priority review — tick this box and the review moves to the front of the queue.

knst
knst approved these changes Sep 8, 2026
View reviewed changes

@knst knst left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 71410d5

@knst
knst merged commit 08525d6 into dashpay:develop Sep 8, 2026
39 checks passed
@PastaPastaPasta
PastaPastaPasta deleted the fix/wallet-locked-coins-handler branch September 8, 2026 15:12
@knst knst added this to the 24 milestone Sep 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@knst knst knst approved these changes
@UdjinM6 UdjinM6 Awaiting requested review from UdjinM6 UdjinM6 is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

24

Development

Successfully merging this pull request may close these issues.

3 participants

@PastaPastaPasta @thepastaclaw @knst